Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PoC: support OCaml 5.3's keywords entry in OCAMLPARAM #535

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dra27
Copy link

@dra27 dra27 commented Nov 5, 2024

This is a little proof-of-concept and request for feedback following a check in ocaml/opam-repository#26831 that the mechanism added in 5.3.0~beta1 in ocaml/ocaml#13471 is working. The effect (pun intended) is mildly limited for 5.3, but it seems worth checking these packages now because keywords changes in future may potentially have more impact, so it feels to me to be better to be ready with a "simpler" release.

The mechanism added allows old code (i.e. old releases of packages) to be compiled even if that code uses identifiers which have become keywords since it was written. This is done either by explicitly passing -keywords 5.2 to the compiler invocations or by setting OCAMLPARAM to _,keywords=5.2. Certainly for the effect keyword, it's generally expected that repositories will choose to switch the identifier (e.g. to effect_) rather than use the -keywords flag, but the mechanism means that opam packages can be made to work without requiring a release, which obviously reduces adoption friction for new versions of the compiler.

This works well for the small number of packages in 5.3 which use effect as an identfier, but it doesn't work for Irmin (cf. mirage/irmin#2346). The OCAMLPARAM trick doesn't work because the ppx standalone driver doesn't inspect OCAMLPARAM, so it's creating the "wrong" AST. This PR is a possible idea to allow the standalone driver (which is invoking the compiler driver, albeit via compiler-libs) to parse the keywords part of OCAMLPARAM.

This Dockerfile fails with OCaml 5.3:

FROM ocaml/opam:debian-12-ocaml-5.3
RUN git clone https://github.com/mirage/irmin.git
WORKDIR irmin
RUN git checkout 3.9.0
RUN sudo ln -f /usr/bin/opam-2.2 /usr/bin/opam
RUN opam install ./irmin.opam --deps-only
RUN opam install repr ppx_irmin mtime bheap
ENV OCAMLPARAM=_,keywords=5.2
RUN opam exec -- dune build -p irmin

giving:

(cd _build/default && .ppx/851ddbf37b8928057d14b915d5ceebb6/ppx.exe --lib Type --cookie 'library-name="irmin"' -o src/irmin/node_intf.pp.ml --impl src/irmin/node_intf.ml -corrected-suffix .ppx-corrected -diff-cmd - -dump-ast)
File "src/irmin/node_intf.ml", line 127, characters 7-13:
127 |   type effect := expected_depth:int -> node_key -> t option
             ^^^^^^
Error: Syntax error

but if this branch is instead pinned then it builds. I'm not necessarily advocating this being the exact mechanism - the only aim is that it should be possible to "patch" an existing compiler simply by setting an environment variable (this is the general intention of OCAMLPARAM and the very obscure $(ocamlc -where)/ocaml_compiler_internal_params file)

  • Incompatibly added at the moment, making ppxlib 5.3+ only!
  • Probably worth including a -keywords option equivalent to the compiler's (In which case, the _ part of OCAMLPARAM should arguably be parsed)?
  • The splitting logic duplicates internal code from Compenv which, if wanted, should probably be exposed in compiler-libs instead
  • I've probably violated the way Clflags should actually be accessed, but I was being a bit lazy trying to work out all the ocaml-compiler-libs stuff...!

@NathanReb
Copy link
Collaborator

NathanReb commented Nov 6, 2024

This looks like a good approach. Fixing the compat with older compilers shouldn't be too hard as we can make the function a no-op on older compilers.

Since it uses compiler-libs directly, it should be moved to astlib. You can use the (* IF_AT_LEAST 503 <ocaml-code> *) syntax, similarly to what's done here: https://github.com/ocaml-ppx/ppxlib/blob/main/astlib/location.ml#L89.

I'm of course happy to take over this if you'd prefer to focus on other things, I assume you're quite busy with the 5.3 release closing in!

@dra27
Copy link
Author

dra27 commented Nov 6, 2024

Ah, thanks for the pointer - it at least brings what I've written to the same state of CI as the main branch! Happy to hand this off or continue working on it - what do you think about having a -keywords flag?

@dra27 dra27 marked this pull request as ready for review November 6, 2024 17:10
@NathanReb
Copy link
Collaborator

I'm happy to add the flag! Can't hurt and it will be documented in the driver's help making this compat feature more easily discovered by ppx users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants